feature(measurement): allowing measurements under `crypto_nossl` feature #250

jakubDoka · 2024-11-19T18:26:12Z

Make the measurement module compatible with the crypto_nossl feature, so openssl feature is not required.

src/certs/snp/ca/mod.rs

src/measurement/gctx.rs

src/measurement/sev_hashes.rs

tylerfanelli · 2024-11-19T19:18:54Z

Also, could you please provide a more in-depth commit title and description, as well as a Signed-off-by to the commit?

tylerfanelli · 2024-11-20T02:54:24Z

Yea, I guess it was exhaustive and not necessary due to the parent module already having the any(openssl, crypto_nossl), but i guess it doesn't hurt. Please squash the commits and otherwise LGTM.

Signed-off-by: Jakub Doka <[email protected]> Update src/measurement/sev_hashes.rs Co-authored-by: Tyler Fanelli <[email protected]> Signed-off-by: Jakub Dóka <[email protected]> Update src/measurement/gctx.rs Co-authored-by: Tyler Fanelli <[email protected]> Signed-off-by: Jakub Dóka <[email protected]> Update gctx.rs Signed-off-by: Jakub Dóka <[email protected]> Update sev_hashes.rs Signed-off-by: Jakub Dóka <[email protected]>

DGonzalezVillal · 2024-11-22T16:51:17Z

Could you also enable the measurement testing under the crypto_nossl flag?

it should be under tests/measurement.rs. That way we can confirm the tests are passing with the changes you committed.

DGonzalezVillal

Just nit pick, once you fix the testing, we can make sure it is working as intended and then you should be good!

DGonzalezVillal · 2024-11-22T17:42:36Z

src/measurement/gctx.rs

@@ -12,6 +13,14 @@ use crate::{
    measurement::snp::{SnpLaunchDigest, LD_BYTES},
 };

+#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]


The library won't compile if both crypto_nossl and openssl are both passed as features, so you can just gate under crypto_nossl

Suggested change

#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]

#[cfg(feature = "crypto_nossl")]

This was my mistake, apologies @jakubDoka

DGonzalezVillal · 2024-11-22T17:45:53Z

src/measurement/sev_hashes.rs

@@ -18,6 +20,14 @@ use crate::error::*;

 type Sha256Hash = [u8; 32];

+#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]


Same thing

Suggested change

#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]

#[cfg(feature = "crypto_nossl")]

Again, my mistake. I asked @jakubDoka to add this forgetting that it won't compile with both features.

DGonzalezVillal · 2024-11-22T17:47:21Z

And you still need to sign-off the commit

DGonzalezVillal · 2024-11-22T18:00:18Z

tests/measurement.rs

@@ -363,7 +363,7 @@ mod snp_tests {
    }
 }

-#[cfg(all(target_os = "linux", feature = "sev"))]
+#[cfg(all(target_os = "linux", feature = "sev", feature = "openssl"))]


Did you only add support for SNP?

Yes I guess the commit title says otherwise, I can add that too

Not the biggest deal. Legacy SEV support will be deprecated soon

jakubDoka · 2024-11-22T18:12:23Z

https://github.com/virtee/sev/blob/main/src/lib.rs#L89

here I see you are enforcing xor featuring, that can be incredibly annoying if you use this crate indirectly and other features end up enabled out of your control, the crate features should just work together no matter the combination you choose

…ng crypto_nossl for sev Signed-off-by: Jakub Doka <[email protected]>

DGonzalezVillal · 2024-11-22T19:26:59Z

https://github.com/virtee/sev/blob/main/src/lib.rs#L89

here I see you are enforcing xor featuring, that can be incredibly annoying if you use this crate indirectly and other features end up enabled out of your control, the crate features should just work together no matter the combination you choose

When the crypto_nossl feature was implemented, it essentially just re-wrote functions that used openssl to use an alternative. To avoid redundancy in code the decision was you had to pick if you were going to use openssl or if you were going to use no_ssl, but you can't compile both at the same time. It makes sense although because why would you want 2 features that do the same thing? It also reduces package dependencies at compile time and avoids other issues. What features are being enabled out of your control?

jakubDoka · 2024-11-22T22:10:05Z

When the crypto_nossl feature was implemented, it essentially just re-wrote functions that used openssl to use an alternative. To avoid redundancy in code the decision was you had to pick if you were going to use openssl or if you were going to use no_ssl, but you can't compile both at the same time. It makes sense although because why would you want 2 features that do the same thing? It also reduces package dependencies at compile time and avoids other issues. What features are being enabled out of your control?

Let's say you depend on a crate A that uses sev and since it does not need to use any openssl-only features, the author decides to use crypto_nossl, but then you also import crate B, this crate is using some features of the sev that require openssl thus you end up with both openssl and crypto_nossl being enabled because rust will not compile the crate for each set of features it's used with, it takes a union of features from all imports. Once you get to this configuration, you will get the compiler error from a dependency and your only saving grace is either, the author of a crate A offers you to switch to the openssl feature, or not use either of A or B.

Another scenario where this is annoying is when you have .eg wasm and native crates in your project and since openssl does not work in that environment, you are forced to use crypto_nossl, then if you still need to use openssl in the native crate, rust-analyzer gets bricked on the compiler error and you need to the do conditional imports in your cargo toml to pick different features for native and wasm even though the crate is never compiled to native code.

Note: if it's not yet possible to compile this crate to wasm, I have an interest in making that possible, but first things need to not depend on OpenSSL

tylerfanelli · 2024-11-23T03:00:23Z

So you're suggesting that we allow both openssl and crypto_nossl features concurrently (ie removing the compiler error) in this case? I'm not totally against it, but I would expect that if both are enabled, one of them (IMO openssl) would take precedent. That is, if you enable both crypto_nossl AND openssl, it would essentially be the same as enabling openssl only.

But I guess this would still break your use case when compiling to wasm as you mentioned above. Is this true?

jakubDoka · 2024-11-23T11:51:36Z

No, in the case of compiling to the Wasm example, this is only an issue when using a workspace and Rust analyzer. Once you build the Wasm crate, you need to make sure OpenSSL is not enabled, which is the case because you have to compile each architecture separately. It's just that rust-analizer runs cargo check for all crates in the workspace

jakubDoka · 2024-11-28T13:28:46Z

@tylerfanelli In any case, I can still apply the changes @DGonzalezVillal is asking for, xor featuring is not that important, I mainly want to move on with this. I have more PRs planned as I need snp measurement and guest side of attestation to be cross-platform (macOS, Windows, WASM if possible)

ghe0 · 2024-12-27T14:18:33Z

Hello!

Can you please let us know which modifications are still needed to close this PR?

larrydewey · 2025-01-13T15:27:45Z

src/measurement/sev.rs

+    #[cfg(feature = "openssl")]
+    {
+        Ok(launch_hash.finish())
+    }


Attribute macros do not allow you to define scope in this way, so this won't actually work. If you are attempting to check at run-time, you will have to use a declarative function-like macro (cfg!), instead.

larrydewey · 2025-01-13T16:20:17Z

src/measurement/sev.rs

+    #[cfg(feature = "openssl")]
+    {
+        Ok(launch_hash.finish())
+    }


Same applies here. Attribute macros have no concept of scope in this way.

jakubDoka requested review from tylerfanelli, larrydewey and DGonzalezVillal as code owners November 19, 2024 18:26

auto-assign bot assigned larrydewey and tylerfanelli Nov 19, 2024

tylerfanelli requested changes Nov 19, 2024

View reviewed changes

src/certs/snp/ca/mod.rs Show resolved Hide resolved

src/measurement/gctx.rs Outdated Show resolved Hide resolved

src/measurement/sev_hashes.rs Outdated Show resolved Hide resolved

jakubDoka force-pushed the main branch from cde9b3c to 5152a83 Compare November 19, 2024 19:40

jakubDoka requested a review from tylerfanelli November 19, 2024 19:45

jakubDoka force-pushed the main branch from f7f8fc5 to 7b4ec62 Compare November 20, 2024 07:12

DGonzalezVillal reviewed Nov 22, 2024

View reviewed changes

enabling the tests for measurement for crypto_nossl and also supporti…

8adc5c6

…ng crypto_nossl for sev Signed-off-by: Jakub Doka <[email protected]>

jakubDoka force-pushed the main branch from 55ea98a to 8adc5c6 Compare November 22, 2024 18:25

larrydewey requested changes Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(measurement): allowing measurements under `crypto_nossl` feature #250

feature(measurement): allowing measurements under `crypto_nossl` feature #250

jakubDoka commented Nov 19, 2024 •

edited

Loading

tylerfanelli commented Nov 19, 2024

tylerfanelli commented Nov 20, 2024

DGonzalezVillal commented Nov 22, 2024

DGonzalezVillal left a comment

DGonzalezVillal Nov 22, 2024

tylerfanelli Nov 23, 2024

DGonzalezVillal Nov 22, 2024

tylerfanelli Nov 23, 2024 •

edited

Loading

DGonzalezVillal commented Nov 22, 2024

DGonzalezVillal Nov 22, 2024

jakubDoka Nov 22, 2024

DGonzalezVillal Nov 22, 2024

jakubDoka commented Nov 22, 2024 •

edited

Loading

DGonzalezVillal commented Nov 22, 2024

jakubDoka commented Nov 22, 2024 •

edited

Loading

tylerfanelli commented Nov 23, 2024 •

edited

Loading

jakubDoka commented Nov 23, 2024

jakubDoka commented Nov 28, 2024

ghe0 commented Dec 27, 2024

larrydewey Jan 13, 2025

larrydewey Jan 13, 2025

	#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]
	#[cfg(feature = "crypto_nossl")]

		@@ -18,6 +20,14 @@ use crate::error::*;

		type Sha256Hash = [u8; 32];

		#[cfg(all(not(feature = "openssl"), feature = "crypto_nossl"))]

feature(measurement): allowing measurements under crypto_nossl feature #250

Are you sure you want to change the base?

feature(measurement): allowing measurements under crypto_nossl feature #250

Conversation

jakubDoka commented Nov 19, 2024 • edited Loading

tylerfanelli commented Nov 19, 2024

tylerfanelli commented Nov 20, 2024

DGonzalezVillal commented Nov 22, 2024

DGonzalezVillal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerfanelli Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

DGonzalezVillal commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubDoka commented Nov 22, 2024 • edited Loading

DGonzalezVillal commented Nov 22, 2024

jakubDoka commented Nov 22, 2024 • edited Loading

tylerfanelli commented Nov 23, 2024 • edited Loading

jakubDoka commented Nov 23, 2024

jakubDoka commented Nov 28, 2024

ghe0 commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feature(measurement): allowing measurements under `crypto_nossl` feature #250

feature(measurement): allowing measurements under `crypto_nossl` feature #250

jakubDoka commented Nov 19, 2024 •

edited

Loading

tylerfanelli Nov 23, 2024 •

edited

Loading

jakubDoka commented Nov 22, 2024 •

edited

Loading

jakubDoka commented Nov 22, 2024 •

edited

Loading

tylerfanelli commented Nov 23, 2024 •

edited

Loading